Fixed optional typing on non-serializable types #2939
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add Documentation and Community
|
github-actions
bot
added
internal
already had some, are any obvious ones missing? |
| value=(annotation, ...), | ||
| ) | ||
| validation_model_class(value=value) | ||
| except Exception: |
There was a problem hiding this comment.
Should this be
try:
...
except ValidationError:
raise
except Exception:
...instead? In case pydantic fails with a regular validation error, we actually want to raise that because it means the types don't match right?
| validation_model_class(value=value) | ||
| except Exception: | ||
| # If Pydantic can't handle it, fall back to isinstance | ||
| if not isinstance(value, annotation): |
There was a problem hiding this comment.
Also this might fail with the raw annotation:
isinstance(int, Union[int, str])
# TypeError: Subscripted generics cannot be used with class and instance checks
There was a problem hiding this comment.
Actually I have a more general question regarding this PR:
This is not a json-serializable object, which means by our definition it can never be a parameter. This means this whole PR is related to a step input, which is of a non-json serializable type, for which there is a None default value and no artifact is being passed to it. Is that correct?
If yes, there might be other simpler solutions to prevent this case from even getting to this parameter validation method.
|
I'm still not entirely sure how to handle this. On the one hand, it would be nice to have your code work as is. On the other hand, if someone now we're to switch the default value to something Non-Null, we would fail and say a parameter has to be JSON-serializable, which also seems weird. In case we want this though, I think we could achieve this by allowing arbitrary types in case the value is def _validate_parameter_input_value(
self, parameter: inspect.Parameter, value: Any
) -> None:
arbitrary_types_allowed = value is None
config_dict = ConfigDict(arbitrary_types_allowed=arbitrary_types_allowed)
... |
Describe changes
This code:
Was failing with the following error message:
def two(a: Optional[int] = None, nn: Optional[NearestNeighbors] = None):While primitive types allowed for optional typing, non-serializable types like NearestNeighbors lead to pydantic errors, as the utility functions were only checking serializable types (as the assumption probably was that at this point we are handling only parameters, not artifacts).
Pre-requisites
Please ensure you have done the following:
developand the open PR is targetingdevelop. If your branch wasn't based on develop read Contribution guide on rebasing branch to develop.Types of changes